Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lnwallet: Make available balance HTLC fee aware #3691

Merged
merged 9 commits into from
Feb 20, 2020

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Nov 7, 2019

This PR makes the balance calculation take into account the extra fee needed to be paid by the initiator when sending non-dust HTLCs. This is a case that could previously be hit both if we as an initiator couldn't pay the HTLC fee, and when the remote (as initiator) couldn't pay to add our HTLC to the commitment.

This PR mitigates this by in those cases reporting a lower number, ensuring the pathfinding algorithm won't attempt to use the channel in such cases.

Fixes #3787.

@halseth
Copy link
Contributor Author

halseth commented Nov 7, 2019

Unit tests not yet pushed

lnwallet/channel.go Outdated Show resolved Hide resolved
htlcswitch/interfaces.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour htlcswitch incomplete WIP PR, not fully finalized, but light review possible P3 might get fixed, nice to have labels Nov 9, 2019
@halseth halseth added this to the 0.9.1 milestone Jan 8, 2020
@halseth halseth force-pushed the link-bandwitch-amt-aware branch 2 times, most recently from 4f01c31 to a5bd181 Compare February 3, 2020 13:51
@halseth halseth changed the title Make link bandwidth amount aware lnwallet: Make available balance HTLC fee aware Feb 3, 2020
@halseth halseth marked this pull request as ready for review February 3, 2020 15:06
@halseth
Copy link
Contributor Author

halseth commented Feb 3, 2020

This is ready for review. (ignore failing travis for now, need to amend unit tests that have expectations about available balance)

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed my initial pass. I understand the high level issue at hand, and the proposed solution, but in my mind this doesn't yet match the implementation in this PR. However it's possible I'm missing some additional context, I've asked some clarifying questions in-line.

lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel.go Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel.go Outdated Show resolved Hide resolved
lnwallet/channel.go Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel.go Outdated Show resolved Hide resolved
lnwallet/channel.go Show resolved Hide resolved
lnwallet/channel.go Outdated Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel.go Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
// If our balance is high enough to pay for a non-dust HTLC and
// the resulting commitment fee, subtract this commitment fee
// to reflect what we have left for such payments.
case ourBalance >= htlcCommitFee+nonDustHtlcAmt:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a gap here when ourBalance == htlcCommitFee+nonDustHtlcAmt - 1? That in that case we only subtract baseCommitFee and the remainder is still enough to create a non dust htlc?

// If we are the channel initiator, we must to subtract the commitment
// fee from our available balance.
if lc.channelState.IsInitiator {
ourBalance -= baseCommitFee
switch {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't some kind of check for the local commitment transaction be added in here too? Similar to the earlier validations that were added to AddHtlc and ReceiveHtlc. So not only make sure we can afford adding it, but also that the remote can afford it if we are the non-initiator?

lnwallet/channel.go Outdated Show resolved Hide resolved
lnwallet/channel.go Show resolved Hide resolved
lnwallet/channel.go Show resolved Hide resolved
@joostjager
Copy link
Contributor

Tested PR and can confirm that I now get a single insufficient local balance failure instead of a never-ending loop.

lnwallet/channel.go Outdated Show resolved Hide resolved
lnwallet/channel.go Outdated Show resolved Hide resolved
@halseth
Copy link
Contributor Author

halseth commented Feb 17, 2020

Pushed new update where channel reserve calculation is moved into availableBalance and we now report a 0 balance for the initiator if only the HTLC fee is left, to avoid getting into the unusable channel situation. PTAL.

bobChannel.channelState.RemoteChanCfg.ChanReserve = aliceMinReserve

// Now let Bob attempt to add an HTLC of 0.1 BTC. He has plenty of
// money, but Alice, which is the initiator, cannot afford any more
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, plenty of money? Bob just has commitFee above his reserve?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He has 5 BTC. Don't know about you, but I think that's plenty of money :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alice also has plenty of money with that definition. I mean, it is about amount spendable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bob has 5 BTC on his end of the channel when it's created. Does that clear things up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanded the comment. Hopefully more clear now.

lnwallet/channel_test.go Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel_test.go Outdated Show resolved Hide resolved
lnwallet/channel.go Outdated Show resolved Hide resolved
lnwallet/channel_test.go Show resolved Hide resolved
lnwallet/channel_test.go Show resolved Hide resolved
// sending yet another HTLC. In practice this can also happen if a fee
// update eats into Alice's balance.
htlcAmt = 1
sendHtlc(htlcAmt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm is it good that this is still possible if there is no amount sendable anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be changing AddHTLC to not allow sending HTLCs even though there is balance left. IMO it is a nice upper layer heuristic to not attempt this by reporting this balance not being available. We can revisit if this becomes a hard protocol requirement, re lightning/bolts#728

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could lead to getting caught in the danger zone if there are concurrent payments. Both payments used the same reported availableBalance, which was enough at that point, but when executed the second payment pushes us below the threshold. If AddHTLC would fail, this wouldn't happen and a new path for the second payment could be searched for.

But this is arguably a rare condition, especially because we plan to do non-concurrent pathfinding for mpp. Maybe it is worth a comment near the AddHTLC implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, does this mean that you can still go below the threshold with SendToRoute?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SendToRoute will still eventually send an HTLC through the link, so none of these checks are bypassed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SendToRoute will also be forwarded by the switch, and Bandwidth is called here:

if amt > l.Bandwidth() {

lnwallet/channel.go Show resolved Hide resolved
@halseth
Copy link
Contributor Author

halseth commented Feb 17, 2020 via email

@halseth halseth force-pushed the link-bandwitch-amt-aware branch 2 times, most recently from 65fa022 to 4a2bde1 Compare February 18, 2020 12:57
Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Tested PR, works as expected. The initiator cannot get itself stuck anymore.

Will do a final pass after second reviewer review

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one modification request here (typo), the rest of the comments either add some commentary for future follow up work, or attempt to resolve lingering unresolved comments.

Ideally this is the last time we add additional heuristics or checks in this area w.r.t HTLC accounting, as things are really starting to stack up. Thankfully we have a pretty good commit trail and set of comments here, so it should be easy to re-asses our assumptions in the future if we need to.

In the end, there's some pretty fundamental fixes here, so that itself alone warrants moving forwarding with the fix. Albeit, imo the code in this area is starting to get a bit cluttered since we're now layering on heuristic after heuristics when the prior is found to be lacking, similar to a game of whack-a-mole.

TBH, I anticipated a number of issues (not these explicit ones of course) years ago when we start to draft the HTLC accounting logic in the spec. However then, the issues seemed more distant as our main goal was just to get something up and deployed that mostly worked in order to prove that this entire thing was even possible. Nevertheless, I think it has become apparent to us all at this point that we may require some more fundamental fixes here to just attempt to eliminate certain classes of these edge cases all together.

LGTM 🛸

bobChannel.channelState.RemoteChanCfg.ChanReserve = aliceMinReserve

// Now let Bob attempt to add an HTLC of 0.1 BTC. He has plenty of
// money, but Alice, which is the initiator, cannot afford any more
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bob has 5 BTC on his end of the channel when it's created. Does that clear things up?

htlcswitch/link.go Show resolved Hide resolved
htlcswitch/link.go Show resolved Hide resolved
feePerKw := filteredView.feePerKw
baseCommitFee := lnwire.NewMSatFromSatoshis(
feePerKw.FeeForWeight(commitWeight),
htlcCommitFee := lnwire.NewMSatFromSatoshis(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the predictAdded field in validateCommitmentSanity, shouldn't we only conditionally add this weight if we're attempting to look ahead by one HTLC to see the expected balance after this HTLC is added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is the assumption that this method is only used atm by the switch when it is attempting to add a new HTLC to the commitment transaction?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non blocking, more of a side point, but if we do add this param, then we can use this to display the current balance of a channel in ListChannels and WalletBalance, if we opt to just mask the concept of a channel reserver all together. If we go this route, then at least the user's wallet will show zero satoshis, rather than showing a balance which can't actually be sent/received (though there're other implications).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is the assumption that this method is only used atm by the switch when it is attempting to add a new HTLC to the commitment transaction?

Yeah, that is the assumption, that it returns the "balance available for HTLCs". And that has to include the HTLC fee if we are the initiator.

// sending yet another HTLC. In practice this can also happen if a fee
// update eats into Alice's balance.
htlcAmt = 1
sendHtlc(htlcAmt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SendToRoute will still eventually send an HTLC through the link, so none of these checks are bypassed.

add

To ba able to validate the commitment sanity both for remote and local
commitments, and at the same time predict both our and their add, we let
validateCommitmentSanity take an extra payment descriptor to make this
possible.
This commit adds a test that was previously not performed, namely that
adding a HTLC would dip the remote initiator below its channel reserve.
This commit adds an extra validation step when adding HTLCs. Previously
we would only validate the remote commitment resulting from adding an
HTLC, which in most cases is enough. However, there are situations where
the dustlimits are different, which could lead to the resulting remote
commitment from adding the HTLC being valid, but not the local
commitment.

Now we also validate the local commitment. A test to trigger the case is
added.
Since we want to handle the edge case where paying the HTLC fee would
take the initiator below the reserve, we move the subtraction of the
reserve into availableBalance where this calculation will be performed.
When we send non-dust HTLCs as the non-initiator, the remote party will
have to pay the extra commitment fee. To account for this we figure out
if they can afford paying this fee, if not we report that we only have
balance available for dust HTLCs, since these HTLCs won't increase the
commitment fee.
…alculation

Since our HTLC must also be added to the remote commitment, we do the
balance caluclation also from the remote chain perspective and report
our minimum balance from the two commit views as our available balance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features / behaviour htlcswitch incomplete WIP PR, not fully finalized, but light review possible P3 might get fixed, nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Take channel reserve into account within pathfinding
3 participants